Add Fish support for environments #15503

AbrilRBS · 2024-01-23T12:06:36Z

Changelog: Feature: Add Fish shell command support for environments
Docs: conan-io/docs#3690

Still need to check if the -g scope for the variable erasure is ok, and how activating both :powershell and :fish confs work

memsharded

Looks good, just I agree it would need a bit more testing of functionality and possible interactions with other shells.

uilianries · 2024-03-22T11:52:50Z

Well, I'm officially converted to the cult of Fish now, so I'm interested as well.

I'll take a look with Ruben to push this PR again.

…fish-support

Signed-off-by: Uilian Ries <[email protected]>

uilianries · 2024-04-01T09:31:46Z

conan/tools/env/environment.py

+            value = varvalues.get_str("${name}", self._subsystem, pathsep=self._pathsep)
+            value = value.replace('"', '\\"')
+            if value:
+                result.append('set -gx {} "{}"'.format(varname, value))
+            else:
+                result.append('set -ge {}'.format(varname))


I guess it could be simplified by using set -p (prepend), instead of expanding all values.

https://fishshell.com/docs/current/cmds/set.html

conans/test/functional/toolchains/env/test_virtualenv_fish.py

memsharded

This is a bit risky in its current form, because it breaks when applied to dependencies or conan build

memsharded · 2024-04-01T20:40:45Z

conan/tools/env/environment.py

+                result.append('set -ge {}'.format(varname))
+
+        content = "\n".join(result)
+        content = relativize_generated_file(content, self._conanfile, "$script_folder")


Isn't it possible to not use this and relativize paths as the other environment do?

memsharded · 2024-04-01T20:43:28Z

conan/tools/env/environment.py

        else:  # Need to deduce it automatically
            is_bat = self._subsystem == WINDOWS
            is_ps1 = self._conanfile.conf.get("tools.env.virtualenv:powershell", check_type=bool)
-            if is_ps1:
+            is_fish = self._conanfile.conf.get("tools.env.virtualenv:fish", check_type=bool)


There are no wrappers for .fish files, are they? If there are no wrappers, this will break when set and used to build any dependency.
This should most likely apply only to end-consumer conanfile, not cache dependencies, and only if not used for conan build or anything like that.

Done, please, take a look in the test test_transitive_tool_requires. It generates a tool requirement, then a second recipe consumes it and runs an executable that prints an environment variable. All using Fish.

memsharded · 2024-04-01T20:44:57Z

conan/tools/env/environment.py

+            deactivate = ("""echo "echo Nothing to restore" > {deactivate_file}"""
+                          .format(deactivate_file=deactivate_file))
+        else:
+            deactivate = textwrap.dedent("""\


For new one, I'd try to do directly an in-memory or in-tmp-folder deactivate script rather than a local generated one.

uilianries · 2024-04-02T15:49:02Z

To understand the current error when trying to use conanrun.sh with fish:

⋊> source conanrun.sh 
/private/tmp/foo/conanrunenv-release-armv8.sh (line 1): Unsupported use of '='. In fish, please use 'set script_folder "/private/tmp/foo"'.
script_folder="/private/tmp/foo"
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^
from sourcing file /private/tmp/foo/conanrunenv-release-armv8.sh
	called on line 1 of file conanrun.sh
from sourcing file conanrun.sh
.: Error while reading file '/private/tmp/foo/conanrunenv-release-armv8.sh'

So, it's incompatible with Fish right now. Just to have an idea, pyenv generates both activate and activate.fish.

I'll check if we can do something compatible with both, because it's a really small script that we have.

…fish-support

uilianries · 2024-04-04T13:19:11Z

Add new test to validate consuming a tool requirement using Fish shell. It should be able to load the tool without errors.

Signed-off-by: Uilian Ries <[email protected]>

memsharded · 2024-04-11T17:51:18Z

conan/tools/env/environment.py

    # TODO: This implemantation is dirty, improve it
    for f in filenames:
        f = f if os.path.isabs(f) else os.path.join(env_folder, f)
        if f.lower().endswith(".sh"):
            if os.path.isfile(f) and "sh" in accept:
                f = subsystem_path(subsystem, f)
                shs.append(f)
+        elif f.lower().endswith(".fish"):
+            if os.path.isfile(f) and "fish" in accept:
+                f = subsystem_path(subsystem, f)


I don't think we should be supporting fish at the moment in Windows subsystems, mostly used externally with win_bash

memsharded · 2024-04-11T17:51:51Z

conan/tools/env/environment.py

@@ -62,6 +73,10 @@ def environment_wrap_command(env_filenames, env_folder, cmd, subsystem=None,
    elif shs:
        launchers = " && ".join('. "{}"'.format(f) for f in shs)
        return '{} && {}'.format(launchers, cmd)
+    elif fishs:
+        launchers = " && ".join('. "{}"'.format(f) for f in fishs)
+        print(f"LAUNCHERS: {launchers}")


Suggested change

print(f"LAUNCHERS: {launchers}")

memsharded · 2024-04-11T17:55:12Z

conan/tools/env/environment.py

+            function {{ function_name }}
+                echo "echo Restoring environment"
+                {% for item in vars_unset %}
+                set -e {{ item }}
+                {% endfor %}
+                {% for item, value in vars_restore.items() %}
+                set -gx {{ item }} {{ value }}
+                {% endfor %}
+            end


The restore should work restoring the variables that were defined at activation time, not the variables that were defined at the time of generating the script. It might be a bit more complicated, the activation needs to store the variables in memory or something too, so the restore can use them

memsharded · 2024-04-11T17:55:50Z

conans/client/generators/__init__.py

@@ -167,6 +168,8 @@ def deactivates(filenames):
                bats.append("%~dp0/"+path)
            elif env_script.endswith(".sh"):
                shs.append(subsystem_path(subsystem, path))
+            elif env_script.endswith(".fish"):
+                fishs.append(subsystem_path(subsystem, path))


Same as above, no subsystem for fish initially, IMHO

uilianries · 2024-04-12T14:05:04Z

@memsharded Thank you for your early review. I just talked to @RubenRBS few minutes ago and found other cases to be testes using spaces and some alternatives for deactivation via memory function.

Most of these changes were made to track the creation of conanbuild.sh and how conan_run invokes with wrapper. I have to confess I spent few hours to understand the flow.

Signed-off-by: Uilian Ries <[email protected]>

memsharded

Looks good! I think it is almost ready.

memsharded · 2024-04-15T16:50:34Z

conan/tools/env/environment.py

+        :param file_location: The path to the file to save the fish script.
+        """
+        filepath, filename = os.path.split(file_location)
+        function_name = f"deactivate_{Path(filename).stem}"


Why the function_name is this one? Why not just the group with the random chars? Is there risk of collision of deactivate_xxxx function name?

It follow the common name used by other virtualenv like: conanbuild.sh -> deactivate_conanbuild.sh. So you can actually can call from your terminal directly, like:

source conanbuild.fish ... deactivate_conanbuild

In Fish, functions are exported as a command, so you can run directly.

About the random to avoid collision: In case we have multiple dependencies add bin folder to PATH, we will need to undo the configuration after deactivating it. Usually it would be store in a deactivate script, but here we are using a variable. Actually, the random is just an idea, but could be the build folder name too, because is kind unique name.

Yes, having something deterministic would make more sense, I probably still didn't understand the issue.

memsharded · 2024-04-15T16:51:35Z

conan/tools/env/environment.py

+            {% if item == "PATH" %}
+            set -g conan_{{ group }}_{{ item }} "${{ item }}"
+            fish_add_path -pg "{{ value }}"


There are other variables that are paths too, like PYTHONPATH, and every other var marked as "path", shouldn't they do the same?

Unfortunately fish_add_path is a feature only to PATH: https://fishshell.com/docs/current/cmds/fish_add_path.html

I didn't find a similar feature for custom paths, but I'll check with Rubén tomorrow for insights.

uilianries · 2024-04-15T19:20:59Z

I'm not sure about the logic that I added: In case a variable already exist, it will be prepend to that variable.

Pros: Preserves system variables and variables already configured by another Conan dependency.

Cons: If an user runs conanbuild.fish twice, all variables will be broken as they will be value duplicated.

I still need to fix some important points when manipulating paths, it's not working right now when needing to prepend.

Signed-off-by: Uilian Ries <[email protected]>

uilianries · 2024-04-22T14:33:23Z

Fish is now installed and configured with tests. Time for review!

memsharded · 2024-05-20T16:14:01Z

conans/test/functional/toolchains/env/test_virtualenv_fish.py

+from conans.util.files import save
+
+# INFO: Fish only supports Cygwin and WSL https://github.com/fish-shell/fish-shell?tab=readme-ov-file#windows
+pytestmark = pytest.mark.skipif(platform.system() not in ("Darwin", "Linux"), reason="Fish is well supported only in Linux and MacOS")


Why this syntax and not a normal @pytest.mark.skip?

Because the entire file only work with Linux and MacOS. Otherwise, would need to apply for each test. It's a documented feature in pytest: https://docs.pytest.org/en/latest/example/markers.html#marking-whole-classes-or-modules

memsharded · 2024-05-20T16:18:48Z

conans/client/generators/__init__.py

+                return ". " + " && . ".join('"{}"'.format(s) for s in files)
+            fishs = [it for it in fishs if os.path.exists(it)]
+            if fishs:
+                filename = "conan{}.fish".format(group)


Here it seems it is missing the deactivation function (not script)?

Actually the deactivation is not an script, but a function exported by Fish in the environment. So we can call the deactivate_conanbuild, but there is no file associated. The function is created on-the-fly: https://github.com/conan-io/conan/pull/15503/files#diff-56b36c23589a66b0bd803c545703684c74400cbb7bd8538b8949ed84613e3fc7R550

memsharded · 2024-05-20T16:29:19Z

conan/tools/env/environment.py

+        :param file_location: The path to the file to save the fish script.
+        """
+        filepath, filename = os.path.split(file_location)
+        function_name = f"deactivate_{Path(filename).stem}"


Yes, having something deterministic would make more sense, I probably still didn't understand the issue.

Signed-off-by: Uilian Ries <[email protected]>

AbrilRBS added 2 commits January 23, 2024 13:03

Add Fish support for environments

5f19714

Erase global scope flag

bf5d21e

memsharded reviewed Jan 23, 2024

View reviewed changes

Add fish functional test

b249c6d

uilianries self-assigned this Mar 22, 2024

uilianries added 2 commits April 1, 2024 10:26

Merge branch 'develop2' of https://github.com/conan-io/conan into rr/…

e16bf22

…fish-support

Add missing import

d42b388

Signed-off-by: Uilian Ries <[email protected]>

uilianries reviewed Apr 1, 2024

View reviewed changes

memsharded reviewed Apr 1, 2024

View reviewed changes

Merge branch 'develop2' of https://github.com/conan-io/conan into rr/…

269b4c8

…fish-support

uilianries added 2 commits April 9, 2024 16:22

test: consume tool requires using fish

e703b07

Signed-off-by: Uilian Ries <[email protected]>

Load fish environment

88a48b4

Signed-off-by: Uilian Ries <[email protected]>

memsharded reviewed Apr 11, 2024

View reviewed changes

uilianries added 2 commits April 15, 2024 16:20

Remove windows subsystems for Windows

c60eb5a

Signed-off-by: Uilian Ries <[email protected]>

Add more tests for fish env

1ab3a8d

Signed-off-by: Uilian Ries <[email protected]>

memsharded reviewed Apr 15, 2024

View reviewed changes

uilianries added 6 commits April 16, 2024 15:19

Fix path remove element

70c34ae

Signed-off-by: Uilian Ries <[email protected]>

generate script wrapper

37eab05

Signed-off-by: Uilian Ries <[email protected]>

validate define with spaces

e32b420

Signed-off-by: Uilian Ries <[email protected]>

Add support path with space

8cdfd29

Signed-off-by: Uilian Ries <[email protected]>

update todo

966c631

Signed-off-by: Uilian Ries <[email protected]>

only generates wrappers when needed

dd517c6

Signed-off-by: Uilian Ries <[email protected]>

This comment was marked as resolved.

Sign in to view

uilianries added 2 commits April 16, 2024 18:32

simplify element remove

325fe06

Signed-off-by: Uilian Ries <[email protected]>

Skip fish test in Windows

2d202ed

Signed-off-by: Uilian Ries <[email protected]>

Add more tests for fish env

66ca6a2

Signed-off-by: Uilian Ries <[email protected]>

This was referenced Apr 18, 2024

Add Fish shell as virtualenv conan-io/docs#3690

Open

Install Fish shell in functional tests conan-io/conan_ci_jenkins#97

Merged

uilianries added 2 commits April 22, 2024 15:37

Sort the expected files

f57f32f

Signed-off-by: Uilian Ries <[email protected]>

Add fish into test configuration

4535b74

Signed-off-by: Uilian Ries <[email protected]>

uilianries marked this pull request as ready for review April 22, 2024 14:33

czoido mentioned this pull request Apr 26, 2024

How to instruct Conan to generate conanbuild.csh files if using csh or tcsh as default shell. #16150

Closed

1 task

uilianries added this to the 2.3.0 milestone Apr 29, 2024

memsharded modified the milestones: 2.3.0, 2.4.0 May 6, 2024

memsharded reviewed May 20, 2024

View reviewed changes

AbrilRBS and others added 4 commits May 21, 2024 18:13

Merge branch 'develop2' into rr/fish-support

32125de

Merge branch 'develop2' into rr/fish-support

cd94ce3

Add missing comma

35d682e

Signed-off-by: Uilian Ries <[email protected]>

Adjust imports with new test folder location

d55eff5

Signed-off-by: Uilian Ries <[email protected]>

AbrilRBS modified the milestones: 2.4.0, 2.5.0 Jun 4, 2024

AbrilRBS modified the milestones: 2.5.0, 2.6.0 Jul 1, 2024

AbrilRBS modified the milestones: 2.6.0, 2.7.0 Jul 31, 2024

AbrilRBS modified the milestones: 2.7.0, 2.8.0 Aug 27, 2024

czoido modified the milestones: 2.8.0, 2.9.0 Sep 24, 2024

AbrilRBS modified the milestones: 2.9.0, 2.10.0 Oct 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Fish support for environments #15503

Add Fish support for environments #15503

AbrilRBS commented Jan 23, 2024 •

edited by uilianries

Loading

memsharded left a comment

uilianries commented Mar 22, 2024

uilianries Apr 1, 2024

memsharded left a comment

memsharded Apr 1, 2024

memsharded Apr 1, 2024

uilianries Apr 16, 2024

memsharded Apr 1, 2024

uilianries commented Apr 2, 2024

uilianries commented Apr 4, 2024

memsharded Apr 11, 2024

memsharded Apr 11, 2024

memsharded Apr 11, 2024

memsharded Apr 11, 2024

uilianries commented Apr 12, 2024 •

edited

Loading

memsharded left a comment

memsharded Apr 15, 2024

uilianries Apr 15, 2024

memsharded May 20, 2024

memsharded Apr 15, 2024

uilianries Apr 15, 2024

uilianries commented Apr 15, 2024 •

edited

Loading

This comment was marked as resolved.

uilianries commented Apr 22, 2024

memsharded May 20, 2024

uilianries May 21, 2024

memsharded May 20, 2024

uilianries May 21, 2024

memsharded May 20, 2024

Add Fish support for environments #15503

Are you sure you want to change the base?

Add Fish support for environments #15503

Conversation

AbrilRBS commented Jan 23, 2024 • edited by uilianries Loading

memsharded left a comment

Choose a reason for hiding this comment

uilianries commented Mar 22, 2024

Choose a reason for hiding this comment

memsharded left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uilianries commented Apr 2, 2024

uilianries commented Apr 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uilianries commented Apr 12, 2024 • edited Loading

memsharded left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uilianries commented Apr 15, 2024 • edited Loading

This comment was marked as resolved.

uilianries commented Apr 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AbrilRBS commented Jan 23, 2024 •

edited by uilianries

Loading

uilianries commented Apr 12, 2024 •

edited

Loading

uilianries commented Apr 15, 2024 •

edited

Loading